Repository navigation
chore(mcp): fix stale steps in the MCP tool authoring docs - #111784
trunk-io[bot] merged 3 commits into
Conversation
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review 🦔 PostHog Review reviewed this pull requestFound 0 must fix, 1 should fix, 0 consider. Published 1 finding (view the review). |
🤖 CI report
|
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (5)📝 WalkthroughWalkthroughThe MCP documentation now describes the CLI wrapper as Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The hand-written guide omits a check that helps keep new action tools’ compact names correctly rendered. This is a bounded documentation risk rather than a current runtime failure, so the PR is low risk with that follow-up. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
services/mcp/src/tools/README.md-33-33 (1)
33-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify that
feature_flagsis registered.The previous review found that the feature filter uses
flags, notfeature_flags. If that remains true, clients requesting?features=flagswill not discover tools copied from this example. Check the current filter and catalog; useflagshere or register and testfeature_flags.#!/bin/bash set -euo pipefail rg -n -C 4 'feature_flags|"flags"|normalize.*feature|feature.*normalize' \ services/mcp/src/tools/toolDefinitions.ts \ services/mcp/schema/tool-definitions.json \ services/mcp/README.md \ services/mcp/tests/unit/tool-filtering.test.ts
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
93bec23c-07b8-4d1c-95f9-091540b3bc54
📒 Files selected for processing (3)
.agents/skills/implementing-mcp-tools/SKILL.mdservices/mcp/README.mdservices/mcp/src/tools/README.md
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 5 remain after this review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc8b4c97c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fc8b4c9 to
bda6af8
Compare
Agents follow these docs literally. Several steps led to wrong output or a red CI run. - `mcp_version` is rejected by the strict YAML validator and nothing reads it, so the key and the "Two MCP versions" section are gone. - The authoring flow did not mention the tool schema snapshot refresh or `generate:ui-apps`. CI fails on stale output for both. - The `TRAILING_ACTIONS` link did not resolve from the skill folder. - The README named the single tool `posthog`. It is `exec`. - `src/tools/README.md` described a feature flag example that does not exist, used hyphenated feature ids, and pointed at a moved test path. It now says it covers hand-written tools only and links to the YAML flow for the rest.
Review follow-ups. The hand-written tool example used `feature_flags`, but the feature filter id for flags is `flags`, so a copied tool would be dropped for `?features=flags`. The authoring steps also missed that a tool behind a new `feature_flag` needs the flag in the snapshot test map, or it gets no snapshot and `generate:ui-apps` fails.
The hand-written tools guide sent authors to the repository README, but the MCP feature list lives in services/mcp/README.md.
d11a99f to
32c5974
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
services/mcp/src/tools/README.md-51-51 (1)
51-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the action-tool naming check.
Line 51 covers kebab-case only. When an action tool can render a domain ending in an unknown operation verb, the author must add that verb to
TRAILING_ACTIONSand test it inservices/mcp/tests/unit/instructions.test.ts. Add this check to the authoring steps.As per coding guidelines, “Whenever you add or rename an action tool, check the
TRAILING_ACTIONSset” and cover it inservices/mcp/tests/unit/instructions.test.ts.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
8e68ad52-de49-428d-8c57-c4fe4a61491e
📒 Files selected for processing (2)
.agents/skills/implementing-mcp-tools/SKILL.mdservices/mcp/src/tools/README.md
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 0 remain after this review.
There was a problem hiding this comment.
Approved.
Docs-only change to MCP authoring guides, with no runtime, schema, or CI behavior touched. The remaining bot comments are minor documentation nits.
- coderabbitai[bot], posthog[bot] reviewed the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 0L, 0F substantive, 295L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T0 auto-approve: T0-deterministic (295L, 3F, two-areas, chore) |
| stamphog 2.3.1 | .stamphog/policy.yml @ 32c5974 · reviewed head 32c5974 |
|
This pull request was merged into |
Problem
Agents that follow the MCP tool authoring docs write YAML that fails validation, or skip a step that CI then fails on.
The docs still show
mcp_version, which the strict YAML validator rejects and nothing reads.They also leave out the tool schema snapshot refresh and the UI apps generation step.
Stacked on the codegen fail-loudly PR. Part of a devex program on MCP codegen.
Changes
implementing-mcp-toolsskill: removesmcp_versionand the "Two MCP versions" section. Adds the snapshot refresh andgenerate:ui-appssteps afterhogli build:openapi. Fixes theTRAILING_ACTIONSlink.services/mcp/README.md: the single tool isexec, notposthog. "Adding New Tools" points to the skill and handbook guide first.services/mcp/src/tools/README.md: now covers hand-written tools only. Drops the feature flag example that referenced files and APIs that do not exist, and fixes the feature id format (snake_case) and the test path.Docs only. No restructuring on purpose.
How did you test this code?
Docs change, so no automated tests. Ran
hogli lint:skillsand markdownlint on both READMEs locally.Checked the new steps against
services/mcp/CONTRIBUTING.md,scripts/yaml-config-schema.tsand.github/workflows/ci-mcp.yml.The snapshot step is covered by the unit tests in CI. The CI workflow has no separate snapshot step. The UI apps step has its own check.
Test rationale: No test needed. The change is documentation and the linters above cover its format.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Docs update
This PR is the docs update.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code. Code written by Claude Sonnet 5.5 (claude-sonnet-5-5) subagents, orchestrated by Claude Opus 5.5 (claude-opus-5-5).
/reviewing-with-coderabbit,/writing-pr-descriptions./writing-skillswas read for the skill edit.